Feature/NVMulator by sajjadtamimi · Pull Request #369 · esa-tu-darmstadt/tapasco

sajjadtamimi · 2023-07-17T13:12:40Z

No description provided.

cahz

Thank you for the contribution. I have a couple of change requests related to the toolflow part. Someone else needs to look at the runtime changes.

cahz · 2023-07-18T14:07:22Z

+		return false
+	}
+
+	proc add_nvmulator {} {


This needs to accept arguments and pass it through

Thanks for your review.
Done.

cahz · 2023-07-18T14:09:42Z

+		connect_bd_net [get_bd_pins /memory/mig/c0_ddr4_ui_clk] [get_bd_pins /memory/NVEmulator_0/CLK]
+		connect_bd_net [get_bd_pins /memory/mem_peripheral_aresetn] [get_bd_pins /memory/NVEmulator_0/RST_N]
+
+
+		puts "Adding NVMulator 0 programmer: memory-side"		
+		delete_bd_objs [get_bd_intf_nets /memory/mig_ic_M00_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/NVEmulator_0/M_AXI_MIG] [get_bd_intf_pins /memory/mig/C0_DDR4_S_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/mig_ic/M00_AXI] [get_bd_intf_pins /memory/NVEmulator_0/S_AXI_NV]
+
+		puts "Connecting NVMulator programmer: memory-side"
+		create_bd_cell -type ip -vlnv xilinx.com:ip:smartconnect:1.0 /memory/smartconnect_0
+		set_property -dict [list CONFIG.NUM_CLKS {2} CONFIG.NUM_MI {1} CONFIG.NUM_SI {1}] [get_bd_cells /memory/smartconnect_0]
+		connect_bd_net [get_bd_pins /memory/smartconnect_0/aclk] [get_bd_pins /memory/mig/c0_ddr4_ui_clk]
+		connect_bd_net [get_bd_pins /memory/smartconnect_0/aclk1] [get_bd_pins /memory/design_clk] 
+		connect_bd_net [get_bd_pins  /memory/mem_peripheral_aresetn] [get_bd_pins /memory/smartconnect_0/aresetn]
+		current_bd_instance "/memory"
+		set s_nvm [create_bd_intf_pin -mode Slave -vlnv xilinx.com:interface:aximm_rtl:1.0 "S_NVM"]
+		connect_bd_intf_net [get_bd_intf_pins /memory/S_NVM] [get_bd_intf_pins /memory/smartconnect_0/S00_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/smartconnect_0/M00_AXI] [get_bd_intf_pins /memory/NVEmulator_0/S_AXI]


This is not portable and has too many assumptions on port names. It will only work on the Alveo U280. In the current form, the plugin should not be under platform/common but instead restricted to U280.

I made it general. I tried it on AU250 and the plugin was able to integrate NVMulator as expected.

cahz · 2023-07-18T14:11:41Z

+			exit 1
+		}
+
+		save_bd_design


Please remove debugging commands such as save_bd_design

cahz · 2023-07-18T14:12:12Z

+		puts "Connecting NVMulator programmer: host to memory"
+		connect_bd_intf_net [get_bd_intf_pins /host/M_NVM] [get_bd_intf_pins /memory/S_NVM]
+
+		current_bd_instance


does this command have any function when used without parameter?

Yes, it uses default value (i.e., 0 for read_latency and write_latency).

I was refering to the current_bd_instance

I see. I also updated input for current_bd_instance

cahz · 2023-07-18T14:12:34Z

+
+		current_bd_instance
+	}
+}


Indentation is off

cahz · 2023-07-18T14:13:24Z

+namespace eval nvmulator {
+
+  proc is_nvmulator_supported {} {
+	  puts "AU280"


This output is not meaningful. Either leave it out or add more context.

I removed it.

wirthjohannes · 2023-07-19T09:18:34Z

I had a short look at the runtime changes.
In my opinion these changes should not be in the "standard" runtime, as they are very specific to this NVMulator (which probably won't be used by most people). This is not specific to this change, but a general problem with the runtime, especially as we don't have an equivalent to e.g. the features in the toolflow.
But I still think, that we shouldn't just continue always adding such specific changes to the general runtime. Especially when this probably can be achieved with less specific changes to the runtime, or maybe none at all.

If I understand it correctly, the changes to the runtime basically just configure the NVMulator IP (via an AXI-Lite slave), correct?

I want to propose some alternatives, on how to achieve this:

Add the NVMulator IP to the PEs configured for your composition with a special PE_ID. Then you could just configure it via the existing launch() calls. While this is somewhat hacky, but it would require no changes to the runtime at all.
Extend the runtime with an option to "write" to platform components (or at least some of them). Something like configure_platform_component(string component_name, uint64_t offset, uint64_t data) (and the other direction query_platform_component). This would look at the components specified in the status core and find the correct one via the name. While this of course requires changes to the runtime, they would not be specific to the NVMulator, but also useful for other stuff (e.g. querying ECC status information).

…_nvmulator proc, and Indentation

cahz · 2024-03-26T10:21:17Z

Is there any progress on this PR? I agree with the suggestions of @wirthjohannes, we need a more general approach to get this PR merged.

Add support for NVMulator

d281da2

sajjadtamimi requested a review from cahz July 17, 2023 13:12

cahz reviewed Jul 18, 2023

View reviewed changes

Remove debugging commands, making plugin general, adding input to add…

c478459

…_nvmulator proc, and Indentation

Conversation

sajjadtamimi commented Jul 17, 2023

Uh oh!

cahz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wirthjohannes commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cahz commented Mar 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wirthjohannes commented Jul 19, 2023 •

edited

Loading